Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds commit lint to the CI #1784

Merged
merged 1 commit into from
Dec 4, 2023
Merged

Adds commit lint to the CI #1784

merged 1 commit into from
Dec 4, 2023

Conversation

andresgalante
Copy link
Contributor

@andresgalante andresgalante commented Nov 30, 2023

closes #1310

This is my first time doing any CI work, so even though I've tested it, and it is annoyingly good at detecting mistakes on commit messages, I'd suggest taking a close look before merging it.

It'd also be good to add the same check on the local build; that way, contributors can check errors before submitting a PR 😄

To test it, just send a couple of PRs to my fork

@rdimitrov rdimitrov requested a review from a team November 30, 2023 14:18
@jhrozek
Copy link
Contributor

jhrozek commented Nov 30, 2023

Thanks for the PR!

do you know what config does the action use by default? For some reason I'm not able to configure commitlint locally - I wanted to pipe a couple of commit messages through the linter so see what would get flagged.

@andresgalante
Copy link
Contributor Author

andresgalante commented Nov 30, 2023

@jhrozek, thanks for taking a look. I've tested several cases, and it looks like the default follows Minder's guidelines and should allow setting up semantic release.

I am not 100% sure but I think that this is the default config.

To test it, please open as many PRs as you want on my fork, and you'll see the errors: There are some example PRs open.

If there is any rule that needs to change, we can create a config for it or base it on the other config examples in the commitlint repo.

@andresgalante
Copy link
Contributor Author

Ignore my last comment, I don't know why I thought that Minders guidelines were asking for semantic release commit messages, when they don't 🤦

I'll update this PR with a config that follows Chris Beans recommendations tomorrow 🙏

@jhrozek
Copy link
Contributor

jhrozek commented Dec 1, 2023

First and most importantly - thank you for caring about commit message hygiene, it's important over the long term. About the semantic commit messages, we had different uses in our team and eventually decided to drop them and just squash all commits in a PR together.

The squashing is actually one thing which I'm wondering would trip up the rules - I'm all for blank lines after subject, limiting length and no dot in subject. But if multiple commits are squashed I wonder if they would trip up the rule that asks for explaining the why.

The other thing is the imperative tone in the subject - I think it's a great rule to follow for humans, but I see some bot messages in our commit history that would break the rule (2445449 for example).
But it seems like those can be excluded (https://stackoverflow.com/questions/60194822/how-do-you-configure-commitlint-to-ignore-certain-commit-messages-such-as-any-th)

The first one I'll test soon (I have one PR with two commits pending now), for the second one I wonder if we could remove that rule from the config and re-add it in a subsequent PR to keep your work merging fast?

@andresgalante
Copy link
Contributor Author

I've updated it so it checks for:

✅ Separate subject from body with a blank line
✅ Limit the subject line to 50 characters
✅ Capitalize the subject line
✅ Do not end the subject line with a period
✅ Wrap the body at 72 characters

These two rules are tricky:

  • Use the imperative mood in the subject line
  • Use the body to explain what and why vs. how

We could create a plugin to check for the imperative mood, but for now, I suggest merging this PR to set up some initial rules. We can address the missing imperative mood rule with a dedicated plugin later.

I've also updated the commit message for this PR since it was following the wrong standards 😬 now I know better!

@andresgalante
Copy link
Contributor Author

andresgalante commented Dec 1, 2023

@jhrozek, great idea! I've just added the rule to ignore WIP messages or messages from dependabot.

Thanks for taking the time to review this one. If this is getting more complex than it should with corner-cases like Dependabot or other services, feel free to just close it 😄

@andresgalante
Copy link
Contributor Author

andresgalante commented Dec 1, 2023

It should be ok now. Here are some tests:

Failed PR, fails:
andresgalante#9

Lint free PR, pass:
andresgalante#10

WIP PR, pass:
andresgalante#11

Dependabot PR, pass:
andresgalante#12

Copy link
Contributor

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry about the delay in review!

@jhrozek jhrozek merged commit c5fc32a into mindersec:main Dec 4, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add CI check for commit messages
2 participants